Convert StateT to IndexedStateT #1775

iravid · 2017-07-30T20:29:19Z

Resolves #1773.

I converted this rather mechanically, so please point out any flaws if you notice. The files will be renamed once discussion is done; it's hard to see the diffs once they're renamed.

Points for discussion:

what additional coverage is needed? I added a basic usage test and the contravariant/profunctor laws.
I duplicated the methods in IndexedStateT's companion object to StateTFunctions to keep versions with less type parameters. This helps because you usually need to write them out due to bad inference in for comprehensions. Does this make sense?
I kept transformS around to minimize breakage, but it seems a bit out of place.
The Contravariant instance is on the SA parameter, while the Functor instance is on the A parameter. Compare this to RWST where the Contravariant instance is for the Reader part.
The Profunctor instance is on the SA, SB parameters while on RWST it is on the Reader and result parameters. This seems inconsistent.

kailuowang

I was thinking providing a StateT implemented in IndexedStateT to minimize API breakage, I.e people currently using StateT would need little to none code change in migration.

andyscott · 2017-07-31T04:35:31Z

👍 for keeping a StateT around, even if it's just backed by or aliased to IndexedStateT.

iravid · 2017-07-31T04:48:12Z

Could you have a look at `package.scala`? I added a type alias and kept the StateT companion object around.

…

________________________________ From: andy scott <notifications@github.com> Sent: Monday, July 31, 2017 7:35:32 AM To: typelevel/cats Cc: Itamar Ravid; Author Subject: Re: [typelevel/cats] Convert StateT to IndexedStateT (#1775) 👍 for keeping a StateT around, even if it's just backed by or aliased to IndexedStateT. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#1775 (comment)>, or mute the thread</~https://github.com/notifications/unsubscribe-auth/AARSxJPRTVrqDIQOZtocwIwlAFkNEiE4ks5sTVmUgaJpZM4OnvN_>.

andyscott · 2017-07-31T05:04:58Z

Aah yes. That seems like it'll do the trick! Thanks.

codecov-io · 2017-07-31T05:57:20Z

Codecov Report

Merging #1775 into master will increase coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1775      +/-   ##
==========================================
+ Coverage    95.2%   95.35%   +0.15%     
==========================================
  Files         248      248              
  Lines        4379     4396      +17     
  Branches      118      116       -2     
==========================================
+ Hits         4169     4192      +23     
+ Misses        210      204       -6

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/package.scala	`85.71% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/IndexedStateT.scala	`100% <100%> (ø)`
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.06% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75bc24a...995d451. Read the comment docs.

Resolves typelevel#1773.

iravid · 2017-07-31T18:18:04Z

Coverage should hopefully improve after the last push. Any thoughts about the discussion points above?

kailuowang · 2017-07-31T19:58:33Z

core/src/main/scala/cats/data/StateT.scala

-  def transformF[G[_], B](f: F[(S, A)] => G[(S, B)])(implicit F: FlatMap[F], G: Applicative[G]): StateT[G, S, B] =
-    StateT(s => f(run(s)))
+  def transformF[G[_], B, SC](f: F[(SB, A)] => G[(SC, B)])(implicit F: FlatMap[F], G: Applicative[G]): IndexedStateT[G, SA, SC, B] =
+    IndexedStateT(s => f(run(s)))


not covered, maybe a doctest?

kailuowang · 2017-07-31T19:59:38Z

core/src/main/scala/cats/data/StateT.scala

-  override def ap[A, B](ff: StateT[F, S, A => B])(fa: StateT[F, S, A]): StateT[F, S, B] =
-    StateT[F, S, B]((s: S) =>
+  override def ap[A, B](ff: IndexedStateT[F, S, S, A => B])(fa: IndexedStateT[F, S, S, A]): IndexedStateT[F, S, S, B] =
+    IndexedStateT[F, S, S, B]((s: S) =>


Not covered, which is a bit surprising. Haven't go the time to investigate further.

kailuowang · 2017-07-31T20:05:44Z

My thoughts on your points for discussion.

coverage - added some comment on code.
can we still separate them into two different traits? StateTFunctions and IndexStateTFunctions? IndexStateT companion can inherit both, while you can have an object StateT inherit StateTFunctions, what do you think?
Leave transformS in the StateTFunctions mentioned above?
I'm not sure about the last two either.

iravid · 2017-08-01T06:43:30Z

Thanks for your comments @kailuowang. I'll address the feedback regarding coverage.

Regarding the functions in the companion object, I'll move the common ones into a StateConstructors trait and keep the distinct ones where they are. This should solve most of the duplication.

Regarding transformS - it's currently on the StateT instances themselves, so I don't see how we can work out a solution where it lives on the companion objects.

iravid · 2017-08-04T09:07:14Z

@kailuowang addressed your comments

peterneyens · 2017-08-04T09:08:15Z

tests/src/test/scala/cats/tests/StateTTests.scala

    checkAll("IndexedStateT[ListWrapper, Int, Int, Int]", AlternativeTests[IndexedStateT[ListWrapper, Int, Int, ?]](SA).monoidK[Int])
+    checkAll("IndexedStateT[ListWrapper, Int, Int, Int]", ApplicativeTests[IndexedStateT[ListWrapper, Int, Int, ?]](SA).applicative[Int, Int, Int])


AlternativeTests already runs the tests from ApplicativeTests, is there a reason to run them explicitly?

Hmm thanks. I wasn't aware of that. Was trying to solve the missing coverage for the ap override on the Alternative instance, but I now realize that this is pointless as there shouldn't be explicit overrides for Applicative methods for StateT. @djspiewak removed them in #1735.

So I'm going to just get rid of this override.

I don't think you can remove ap here as it is needed to implement the Alternative instance?

Yes, to solve it I added with IndexedStateTMonad to the instantiation in the instances trait

peterneyens · 2017-08-04T09:11:33Z

core/src/main/scala/cats/data/StateT.scala

- * `StateT[F, S, A]` is similar to `Kleisli[F, S, A]` in that it takes an `S`
- * argument and produces an `A` value wrapped in `F`. However, it also produces
- * an `S` value representing the updated state (which is wrapped in the `F`
- * context along with the `A` value.


Can we move the old StateT documentation to the StateT type alias in package.scala?

iravid · 2017-08-04T12:27:07Z

So how does this look to you @kailuowang @peterneyens? I'll start working on IRWST soon in hopes we could get this merged in time for RC1.

kailuowang · 2017-08-04T14:45:39Z

core/src/main/scala/cats/data/StateT.scala

-  def empty[A]: StateT[F, S, A] =
-    StateT.lift[F, S, A](G.empty[A])(G)
-
-  override def ap[A, B](ff: StateT[F, S, A => B])(fa: StateT[F, S, A]): StateT[F, S, B] =


I am curious why remove this override?

The specific overrides for Applicative were removed in #1735, so my reasoning was that this override wasn't necessary either, and it suffices to use the implementation provided by the Monad instance

I see, you made IndexedStateTAlternative require mixing with a Monad instance trait. Left some comment below.

kailuowang · 2017-08-04T15:27:08Z

core/src/main/scala/cats/data/StateT.scala

-  implicit def catsDataAlternativeForStateT[F[_], S](implicit FM: Monad[F], FA: Alternative[F]): Alternative[StateT[F, S, ?]] =
-    new StateTAlternative[F, S] { implicit def F = FM; implicit def G = FA }
+private[data] sealed trait IndexedStateTInstances extends IndexedStateTInstances1 {
+  implicit def catsDataAlternativeForIndexedStateT[F[_], S](implicit FM: Monad[F], FA: Alternative[F]): Alternative[StateT[F, S, ?]] =


I wonder what if you make this instance an Alternative[StateT[F, S, ?]] with Monad[StateT[F, S, ?]] and let IndexedStateTAlternative inherit IndexedStateTMonad. Since without the ap, IndexedStateTAlternative can't be used without mixin with IndexedStateTMonad
Do you still need to override pure?

Will try :-) would that approach look cleaner? Personally I have no preference.

Definitely cleaner

Make the return type Alternative[StateT[F, S, ?]] with Monad[StateT[F, S, ?]] here as well?

Hey @kailuowang - I actually changed Alternative to extend Monad, so there's no need for the intersection type now.

That's what I meant. Since you are in effect returning an Alternative with Monad, why hide the fact that it's a Monad? It has a lot more power than the return type suggests. I just felt a bit odd that your return type isn't the actual public type of the instance. Odd code take a bit more time for code reader to grasp, that's all.

My reasoning was that this was about implicit search priorities: let the catsDataAlternativeForIndexedStateT return only an Alternative instance, and let the catsDataMonadForIndexedStateT handle the Monad instance, while hiding the fact that the Alternative instance in fact uses the Monad instance.

If you find that returning the intersection type makes more sense, I'll fix that right away :-) Just haven't seen this done anywhere else in the codebase.

We have examples of returning intersection type.
/~https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/instances/list.scala#L11
IIRC, at this position instance use to be a MonadCombine which is MonadFilter with Alternative, so I don't have issues with this instance being a Monad with Alternative

Cool! Thanks. Changed.

Thank you for putting up with my nitpicking! I will find time to go through the whole PR tomorrow or the day after.

kailuowang · 2017-08-04T15:31:15Z

@iravid , I left a comment, will continue on it later (probably sometime next week though)

johnynek · 2017-08-05T18:41:30Z

core/src/main/scala/cats/data/StateT.scala

    transform { case (s, a) => (s, f(a)) }

+  def contramap[S0](f: S0 => SA)(implicit F: Monad[F]): IndexedStateT[F, S0, SB, A] =


doesn't this only need Applicative[F]?

(pure(f), runF).map2(_.andThen(_)) I think does it.

Yes! That's awesome, thank you :-)

Downgraded dimap too

Sorry for the noise. Turns out it's just Functor that's needed for both.

iravid · 2017-08-08T18:04:16Z

Any other comments anyone?

iravid · 2017-08-16T06:29:40Z

Any additional feedback would be appreciated, folks

kailuowang

LGTM, sorry it took so long for me to finish my review.

iravid · 2017-08-31T17:16:23Z

No worries. As @non mentioned in the RWST PR, I'd like to rename the files once reviews are done. So please hold off on merging after another reviewer has finished reviewing :-)

kailuowang · 2017-08-31T20:23:39Z

core/src/main/scala/cats/data/StateT.scala

-private[data] sealed trait StateTInstances extends StateTInstances1 {
-  implicit def catsDataAlternativeForStateT[F[_], S](implicit FM: Monad[F], FA: Alternative[F]): Alternative[StateT[F, S, ?]] =
-    new StateTAlternative[F, S] { implicit def F = FM; implicit def G = FA }
+private[data] sealed trait IndexedStateTInstances extends IndexedStateTInstances1 {


per #1879 do you mind change all the instance traits here into abstract class? I am creating a PR to change other classes, but don't want to touch this one to avoid conflict.

iravid · 2017-09-08T17:53:30Z

Ping. This has been sitting here for a while. Any chance we could move this forward?

kailuowang · 2017-09-08T18:17:34Z

@iravid github notification may not reach the people you want to reach ( they got so many of them). maybe you could ping the two other reviewers in gitter cats-dev?

iravid · 2017-09-08T18:20:26Z

Good point @kailuowang :-)

johnynek

👍
This looks good to me. Thanks for pushing this through and sorry I hadn't looked sooner.

johnynek · 2017-09-10T04:14:14Z

core/src/main/scala/cats/data/StateT.scala

- * an `S` value representing the updated state (which is wrapped in the `F`
- * context along with the `A` value.
+ *
+ * `IndexedStateT[F, SA, SB, A]` is a stateful computation in a context `F` yielding


can you comment about the Indexed name here? I don't know the context, though I can understand the rest of the comments.

Thanks @johnynek. I refined the scaladoc here. See if that makes it more descriptive.

iravid · 2017-09-10T10:13:45Z

@kailuowang Let me know if this is sufficient review-wise and I will rename the files before merge :-)

johnynek · 2017-09-10T19:14:24Z

Okay. Good point about Arrow.

I'd now probably say we shouldn't add it since just using Monoid feels rather lawless to me. Not clear the monoid empty is what you want since we don't combine on these values.

What do you think?

We could keep as you have it, drop Arrow, or make the Arrow def accept the values to use within lift, which means it can't be implicit but at least saves someone the work of implementing it if they want it (although implementing it isn't that much work).

iravid · 2017-09-11T05:01:32Z

@johnynek I would lean towards removing it entirely in that case, but I had never used Arrow and friends before, so I don't have clear intuition here.

We could keep Strong as it does not involve monoids on the value type.

johnynek · 2017-09-11T05:35:02Z

I concur. Sorry for the goose chase here.

On Sun, Sep 10, 2017 at 19:01 Itamar Ravid ***@***.***> wrote: @johnynek </~https://github.com/johnynek> I would lean towards removing it entirely in that case, but I had never used Arrow and friends before, so I don't have clear intuition here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1775 (comment)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AAEJdr7x9JfD_AT8tFMS4NIzH2sFCSqDks5shL6tgaJpZM4OnvN_> .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

iravid · 2017-09-11T05:47:09Z

No worries :-)

________________________________ From: P. Oscar Boykin <notifications@github.com> Sent: Monday, September 11, 2017 8:35:04 AM To: typelevel/cats Cc: Itamar Ravid; Mention Subject: Re: [typelevel/cats] Convert StateT to IndexedStateT (#1775) I concur. Sorry for the goose chase here.

On Sun, Sep 10, 2017 at 19:01 Itamar Ravid ***@***.***> wrote: @johnynek </~https://github.com/johnynek> I would lean towards removing it entirely in that case, but I had never used Arrow and friends before, so I don't have clear intuition here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1775 (comment)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AAEJdr7x9JfD_AT8tFMS4NIzH2sFCSqDks5shL6tgaJpZM4OnvN_> .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1775 (comment)>, or mute the thread</~https://github.com/notifications/unsubscribe-auth/AARSxH4Zd9tXX_pO29iwukO1zbPIOhpOks5shMaIgaJpZM4OnvN_>.

johnynek · 2017-09-11T19:05:26Z

👍

iravid force-pushed the feature/indexed-state branch from 1bbbccf to 9653863 Compare July 30, 2017 20:39

kailuowang reviewed Jul 31, 2017

View reviewed changes

iravid force-pushed the feature/indexed-state branch from 9653863 to c001d9c Compare July 31, 2017 05:57

Convert StateT to IndexedStateT

289c538

Resolves typelevel#1773.

iravid force-pushed the feature/indexed-state branch from c001d9c to 289c538 Compare July 31, 2017 18:17

kailuowang reviewed Jul 31, 2017

View reviewed changes

Itamar Ravid added 3 commits August 4, 2017 11:56

Add doctest for transformF

9ffcbdb

Add law checking for applicative

b2b00a7

Move common StateT constructors to separate trait

f51ba69

peterneyens reviewed Aug 4, 2017

View reviewed changes

Itamar Ravid added 2 commits August 4, 2017 12:30

Remove explicit ap override in StateT's Alternative instance

84e1db0

Add previous StateT documentation to type alias

a005a61

kailuowang reviewed Aug 4, 2017

View reviewed changes

kailuowang added this to the 1.0.0-RC1 milestone Aug 4, 2017

Make IndexedStateTAlternative extend IndexedStateTMonad

3423199

johnynek reviewed Aug 5, 2017

View reviewed changes

Downgrade contramap and dimap to Applicative on F

1315a29

kailuowang mentioned this pull request Aug 7, 2017

Add transformS() in ReaderWriterStateT #1786

Closed

Itamar Ravid added 2 commits August 8, 2017 23:05

Specify full intersection type for returned Alternative instance

49de02f

Add Bifunctor instance

0268e47

iravid mentioned this pull request Aug 9, 2017

Convert ReaderWriterStateT to IndexedReaderWriterStateT #1803

Merged

kailuowang added the Source Breaking label Aug 28, 2017

kailuowang approved these changes Aug 31, 2017

View reviewed changes

kailuowang reviewed Aug 31, 2017

View reviewed changes

Convert instance traits to abstract classes

7ee428a

kailuowang approved these changes Sep 8, 2017

View reviewed changes

johnynek reviewed Sep 10, 2017

View reviewed changes

Itamar Ravid added 2 commits September 10, 2017 08:01

Refine comment on IndexedStateT

3be4238

Add Strong and Arrow instances for IndexedStateT

6e25fbb

iravid force-pushed the feature/indexed-state branch from c6e9c2a to 6e25fbb Compare September 10, 2017 08:02

Rename files

3151311

Remove arrow instance

995d451

iravid force-pushed the feature/indexed-state branch from 53ea464 to 995d451 Compare September 11, 2017 13:40

kailuowang merged commit 5bb0252 into typelevel:master Sep 11, 2017

kailuowang added the enhancement label Dec 14, 2017

		checkAll("IndexedStateT[ListWrapper, Int, Int, Int]", AlternativeTests[IndexedStateT[ListWrapper, Int, Int, ?]](SA).monoidK[Int])
		checkAll("IndexedStateT[ListWrapper, Int, Int, Int]", ApplicativeTests[IndexedStateT[ListWrapper, Int, Int, ?]](SA).applicative[Int, Int, Int])

		transform { case (s, a) => (s, f(a)) }

		def contramap[S0](f: S0 => SA)(implicit F: Monad[F]): IndexedStateT[F, S0, SB, A] =

Convert StateT to IndexedStateT #1775

Convert StateT to IndexedStateT #1775

Conversation

iravid commented Jul 30, 2017 • edited Loading

kailuowang left a comment

Choose a reason for hiding this comment

andyscott commented Jul 31, 2017

iravid commented Jul 31, 2017 via email

andyscott commented Jul 31, 2017

codecov-io commented Jul 31, 2017 • edited Loading

Codecov Report

iravid commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Jul 31, 2017

iravid commented Aug 1, 2017 • edited Loading

iravid commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iravid commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iravid commented Aug 8, 2017

iravid commented Aug 16, 2017

kailuowang left a comment

Choose a reason for hiding this comment

iravid commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iravid commented Sep 8, 2017

kailuowang commented Sep 8, 2017

iravid commented Sep 8, 2017

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iravid commented Sep 10, 2017

johnynek commented Sep 10, 2017

iravid commented Sep 11, 2017 • edited Loading

johnynek commented Sep 11, 2017 via email

iravid commented Sep 11, 2017 via email

johnynek commented Sep 11, 2017

iravid commented Jul 30, 2017 •

edited

Loading

codecov-io commented Jul 31, 2017 •

edited

Loading

iravid commented Aug 1, 2017 •

edited

Loading

kailuowang Aug 4, 2017 •

edited

Loading

iravid commented Sep 11, 2017 •

edited

Loading